Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added filtering of emails and websites #1

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Abstract45
Copy link

Now user can choose to filter only profanity via previous syntax or filter additional content such as website and/or emails.

Copy link
Owner

@ryanmaxwell ryanmaxwell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea for a PR! Unfortunately this breaks Obj-c compatibility (run the tests), as there are few Swift features here that are not compatible with obj-c.

A few suggestions:

  1. Default arguments don't work in obj-c. you must declare the simpler method and then call into the more complex method with the default.

  2. Since you have made public the the string extensions for isValidWebsite() and isValidEmail(), I suggest changing them to computed properties (vars instead of funcs) as this are more consistent with swift 3's isEmpty which is now a var. Or else make them private and put them in an internal utility.

  3. the filter argument is an array of a swift enum. This cannot be represented in obj-c. I would suggest using an option set here. Swift option set types are not bridgeable to obj-c but you can go the other way around. see http://stackoverflow.com/questions/32530018/accessing-swift-optionsettype-in-objective-c.

Also, remember that an option set means you can have ANY of the passed in filters apply. So you can't have if, else if etc logic as you do in the block other function as it will mean only ONE of the filters will apply.

Could you also write some new tests for the added functionality but also keep the original tests as they were to ensure correct regression testing.

- Wrote tests in Objective C and Swift for new filter types
@Abstract45
Copy link
Author

Ok so now it should work in both Swift and Objective C.

  • I used Dictionaries for Objective C and I am using that dictionary to make an array of enums which is working in the tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants